Repository navigation
PERF: avoid repeated text fetch preparation for fetchmany(1) - #891
Conversation
Preserve batch decoding while completing eligible size-one text fetches natively. Add one mixed-text performance-report workload and focused contract coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR Performance Report✅ Performance improved1 database task consistently improved across 2 measured environments. No consistent slowdowns were detected. 1 IMPROVEMENT 0 SLOWDOWNS 2/2 ENVIRONMENTS Signal fingerprint
Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Measured timings
Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. Unix / SQL Server 202210,000 INT/NVARCHAR rows / fetchmany(1): ddbc::NativeCursor::prepare -1111.036 ms; ddbc::NativeCursor::fallback -225.222 ms; py::fetchmany::cpp_call -65.007 ms. Call changes: ddbc::FetchBatchData (added, removed, or intermittent); ddbc::FetchBatchData::SQLFetchScroll_call (added, removed, or intermittent); ddbc::FetchBatchData::cache_column_metadata (added, removed, or intermittent). Unix / SQL Server 202510,000 INT/NVARCHAR rows / fetchmany(1): ddbc::NativeCursor::prepare -968.076 ms; ddbc::NativeCursor::fallback -148.066 ms; py::fetchmany::cpp_call -41.414 ms. Call changes: ddbc::FetchBatchData (added, removed, or intermittent); ddbc::FetchBatchData::SQLFetchScroll_call (added, removed, or intermittent); ddbc::FetchBatchData::cache_column_metadata (added, removed, or intermittent). All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
There was a problem hiding this comment.
🔵 Needs a closer look
Source-only review cannot establish cross-platform/live-SQL correctness of the native fetch changes or attribute the reported profiling-OFF bulk-fetch regressions without clean controls.
No open issues 🎉
What changed in this PR
This PR targets repeated dispatch preparation for text-bearing fetchmany(1) calls in the native driver, addressing part of the fetch-performance concerns raised in #554.
Changes:
- Keep size-one text fetches on native completion, sharing batch decoding for bounded wide text and retaining the batch reader for narrow text.
- Add regression coverage for dispatch counts, text boundaries, converter bytes, malformed text, and garbage-collection invalidation.
- Add one 10,000-row INT/NVARCHAR workload with timing-boundary and report-contract tests, bringing the profiler registry to 26 tasks.
The reported instrumented gains do not establish release-latency improvements. Bulk-fetch regressions remain unresolved, and the PR remains draft pending clean control measurements and cross-platform validation.
| File | Description |
|---|---|
tests/test_single_row_pipeline.py |
Tests wide-text boundaries and updated native-path counts. |
tests/test_output_converter_bytes.py |
Extends converter and malformed-text coverage to fetchmany(1). |
tests/test_native_fetch_dispatch.py |
Tests stable dispatch reuse and garbage-collection invalidation. |
tests/test_036_profiler_ci.py |
Validates the new workload’s timing, retention, cleanup, and reporting. |
mssql_python/pybind/native_fetch.hpp |
Completes size-one text reads natively without repeated policy preparation. |
mssql_python/pybind/fetch_text.hpp |
Extracts the shared batch UTF-16 decoder. |
mssql_python/pybind/ddbc_bindings.h |
Uses the shared decoder for bound wide-text batches. |
mssql_python/pybind/ddbc_bindings.cpp |
Enables bounded wide-text single-row reads with batch decoding semantics. |
mssql_python/_native_fetch.py |
Updates dispatch documentation for size-one text fetches. |
eng/profiler_benchmarks/workloads.py |
Adds and registers fetchmany_one_text. |
eng/profiler_benchmarks/report.py |
Adds the new workload’s report label. |
eng/profiler_benchmarks/README.md |
Documents workload boundaries and updates registry counts. |
Review effort: Balanced
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 3552-3560 3552 (columnSize == 0 || columnSize == SQL_NO_TOTAL || columnSize > SQL_MAX_LOB_SIZE));
3553 }
3554
3555 static inline bool CanUseSingleRowForFetchMany(SQLSMALLINT dataType, SQLULEN columnSize) {
! 3556 // Narrow text still needs the batch's platform/codec-specific conversion.
3557 switch (dataType) {
3558 case SQL_CHAR:
3559 case SQL_VARCHAR:
3560 case SQL_LONGVARCHAR:Lines 3939-3949 3939 text = OutputConverters::text(textData, textSize);
3940 row.append(text ? text
3941 : batchText
3942 ? steal(FetchText::decode_batch_utf16_native(
! 3943 textData, textSize))
! 3944 : FetchText::from_utf16_native(textData,
! 3945 textSize));
3946 LOG("SQLGetData: Appended NVARCHAR string "
3947 "length=%lu for column %d",
3948 (unsigned long)numCharsInData, i);
3949 } else {mssql_python/pybind/native_fetch.hppLines 737-745 737 ret = FetchMany_wrap(plan->handle, rows, 1, plan->charEncoding,
738 plan->wideEncoding, plan->charCtype, plan->at(Messages));
739 if (!current(self, context, state, *plan, rt) ||
740 (!SQL_SUCCEEDED(ret) && ret != SQL_NO_DATA))
! 741 return py::reinterpret_borrow<py::object>(self).attr("_complete_fetchmany")(rows, ret);
742 if (PyList_GET_SIZE(rows.ptr()))
743 values = py::reinterpret_borrow<py::list>(PyList_GET_ITEM(rows.ptr(), 0));
744 else
745 ret = SQL_NO_DATA;📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 64.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python._native_fetch.py: 77.2%
mssql_python.__init__.py: 80.8%
mssql_python.pybind.row_mapping_cache.hpp: 81.5%
mssql_python.pybind.connection.connection_pool.cpp: 82.5%
mssql_python.pybind.ddbc_bindings.cpp: 82.5%
mssql_python.pybind.connection.connection.cpp: 83.1%🔗 Quick Links
|
There was a problem hiding this comment.
🔵 Needs a closer look
Native fetch routing and decoding require live SQL Server and cross-platform validation, while unresolved profiling-OFF bulk-fetch slowdowns need clean control measurements and human review.
No open issues 🎉
Review effort: Balanced
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
Reviewed the PR for correctness, security, reliability, performance, test coverage, repository conventions, and applicable architecture and design specifications. No actionable issues were identified. The implementation is consistent with repository standards and the applicable approved design requirements.
Recommendation: Approve
There was a problem hiding this comment.
🔵 Needs a closer look
Native fetch routing and platform-specific decoding need live SQL Server and cross-platform validation unavailable here, while unresolved profiling-OFF bulk-fetch slowdowns require clean control measurements and human review.
No open issues 🎉
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Reported profiling-OFF bulk-fetch slowdowns and unverified native routing and decoder parity require clean measurements, live SQL Server testing, and cross-platform CI results for human review.
No open issues 🎉
Review effort: Balanced
Collect prior watched cursor cycles before asserting stable native reenable counts. Extend deterministic GC coverage to converter callback removal, preserving exact fetch/fallback counts, metadata ownership, and callback semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2dabb8d
There was a problem hiding this comment.
🔵 Needs a closer look
Native fetch routing and platform-specific decoding require unavailable live-SQL cross-platform validation, while reported profiling-OFF bulk-fetch regressions remain unresolved pending clean control measurements.
No open issues 🎉
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The CHAR/VARCHAR fallback lacks mutation-during-batch-fetch regression coverage because the existing test intercepts SQLFetch_ptr rather than SQLFetchScroll_ptr, leaving exactly-once processing and prevention of skipped or duplicated rows unverified.
1 open issue
Review effort: Balanced
Work Item / Issue Reference
Summary
Keep size-one text fetches on native completion so ordinary row-count updates do not rebuild the dispatch policy for every row. Bounded wide text reuses the established batch decoder; narrow text retains the batch reader. Preserve Row/RowMapping behavior, converter bytes, malformed-text handling, mutation checks and consumed-row completion. No prefetching, weak-state cache or stack-buffer changes.
Add exactly one PR performance workload:
fetchmany_one_text, collecting 10,000 ordered INT/NVARCHAR rows with explicitfetchmany(1). It retains rows through EOF and validates values/types outside timing. Existing comparison thresholds and bulk controls are unchanged. Add one workload/report contract test and update the registry/documentation.Correct the GC-sensitive plain-tuple count test after reproducing its original failure: collection of an older watched cursor during plan preparation legitimately invalidates the global epoch. Keep exact stable-path counts and separately test the required one-read fallback; production invalidation is unchanged.
Validation
python -m pre_commit run black-check --all-files --hook-stage pre-push: passed./fetchstudy/venv/bin/python /fetchstudy/validate_worker.py candidate-on tests: passed, 145 report contract tests. The import-safe runner executespytest.main(["-q", "-ra", "--tb=short", "tests/test_036_profiler_ci.py"])against the fresh candidate build.Draft / remaining performance qualification
Measured base is
730cb2bec05c650b1dba069bf9cbbadb810291d9. Later main01a18f76a58ccf8f23e2c3d3f88b88a37b81ac54changes only the parameter helper and its tests; it was not folded into local measurements. CI must validate the current merge.The separate profiling-OFF study demonstrates the mixed-text regression improvement but does not beat pyodbc or show repeatable fetchone gains. Adverse bulk-fetch controls remain unresolved: the second primary fetchall comparison is candidate/main 1.221, 95% CI [1.060, 1.410]; fetchmany(1000) is also adverse. Identical-binary replicas vary under shared-host contention, but this does not establish that the regressions are harmless. Keep this PR draft pending clean control measurements and cross-platform CI; no regression-free or merge-ready claim.